Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[refactor] #4229: Removed MST aggregation #4308

Merged
merged 1 commit into from
Feb 29, 2024
Merged

[refactor] #4229: Removed MST aggregation #4308

merged 1 commit into from
Feb 29, 2024

Conversation

Stukalov-A-M
Copy link
Contributor

@Stukalov-A-M Stukalov-A-M commented Feb 20, 2024 •
edited
Loading

Description

  • Removed MST aggregation and derived methods
  • Removed hash_of_payload method for Transactions
  • Removed merge_signatures method
  • Removed pending_transactions endpoint and derived methods

Now not fully signed tx will be declined by queue with SignatureCondition error

Linked issue

Benefits

Checklist

  • I've read CONTRIBUTING.md
  • I've used the standard signed-off commit format (or will squash just before merging)
  • All applicable CI checks pass (or I promised to make them pass later)
  • (optional) I've written unit tests for the code changes
  • I replied to all comments after code review, marking all implemented changes with thumbs up

@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Feb 20, 2024
VAmuzing
VAmuzing reviewed Feb 20, 2024
View reviewed changes
client_cli/src/main.rs Outdated Show resolved Hide resolved
core/src/queue.rs Show resolved Hide resolved
@Arjentix Arjentix self-assigned this Feb 20, 2024
@coveralls
Copy link

coveralls commented Feb 20, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 8095893403

Details

  • 38 of 73 (52.05%) changed or added relevant lines in 8 files are covered.
  • 3596 unchanged lines in 60 files lost coverage.
  • Overall coverage decreased (-0.3%) to 56.533%
Changes Missing Coverage Covered Lines Changed/Added Lines %
core/src/block.rs 0 1 0.0%
core/src/sumeragi/main_loop.rs 0 1 0.0%
torii/src/lib.rs 0 1 0.0%
torii/src/routing.rs 0 1 0.0%
core/src/gossiper.rs 0 2 0.0%
core/src/queue.rs 36 41 87.8%
client_cli/src/main.rs 0 7 0.0%
client/src/client.rs 2 19 10.53%
Files with Coverage Reduction New Missed Lines %
core/src/sumeragi/network_topology.rs 1 98.78%
tools/kagami/src/crypto.rs 1 12.12%
data_model/src/metadata.rs 2 78.87%
primitives/src/unique_vec.rs 2 91.24%
config/src/kura.rs 3 80.0%
config/src/logger.rs 5 72.73%
logger/src/lib.rs 5 94.12%
tools/kagami/src/main.rs 5 0.0%
crypto/src/signature/secp256k1.rs 7 96.09%
data_model/src/smart_contract.rs 7 0.0%
Totals Coverage Status
Change from base Build 7884695009: -0.3%
Covered Lines: 22517
Relevant Lines: 39830
💛 - Coveralls

@Erigara Erigara self-assigned this Feb 20, 2024
@Stukalov-A-M Stukalov-A-M force-pushed the removed_mst_aggregation branch 2 times, most recently from 3baf01a to 65d86cc Compare February 20, 2024 21:17
@Stukalov-A-M
Copy link
Contributor Author

@AlexStroke , could you pls. look at client_cli python tests. I removed check_mst flag from args, and assume that cli tests are dropped because of this.

@github-actions github-actions bot added the api-changes Changes in the API for client libraries label Feb 21, 2024
Arjentix
Arjentix reviewed Feb 21, 2024
View reviewed changes
client/tests/integration/multisignature_transaction.rs Outdated Show resolved Hide resolved
client/tests/integration/multisignature_transaction.rs Outdated Show resolved Hide resolved
core/src/queue.rs Outdated Show resolved Hide resolved
core/src/queue.rs Outdated Show resolved Hide resolved
core/src/queue.rs Outdated Show resolved Hide resolved
core/src/tx.rs Show resolved Hide resolved
@Erigara
Copy link
Contributor

Erigara commented Feb 22, 2024 •
edited
Loading

@Stukalov-A-M about pytests, that's right pytests use --skip-mst-check by default

VAmuzing
VAmuzing previously approved these changes Feb 27, 2024
View reviewed changes
@s8sato
Copy link
Contributor

s8sato commented Feb 27, 2024 •
edited
Loading

It seems to me that with this change, Iroha will temporarily lose the feature of multi-signature transactions.
Is that okay for you all?
My concerns are noted here #4229 (comment)

@Stukalov-A-M
Copy link
Contributor Author

It seems to me that with this change, Iroha will temporarily lose the feature of multi-signature transactions. Is that okay for you all? My concerns are noted here #4229 (comment)

We have discussed it with @Arjentix exactly an idea to keep MST via triggers and it seems logical to divide the MST question resolving on 2 steps:

  1. Remove current MST realization
  2. Design a new solution for MST

Erigara
Erigara reviewed Feb 28, 2024
View reviewed changes
core/src/queue.rs Outdated Show resolved Hide resolved
Erigara
Erigara reviewed Feb 28, 2024
View reviewed changes
core/src/queue.rs Outdated Show resolved Hide resolved
@Stukalov-A-M Stukalov-A-M force-pushed the removed_mst_aggregation branch 4 times, most recently from bc293b0 to c8af9dd Compare February 28, 2024 19:38
VAmuzing
VAmuzing reviewed Feb 29, 2024
View reviewed changes
core/src/queue.rs Outdated Show resolved Hide resolved
@Stukalov-A-M Stukalov-A-M force-pushed the removed_mst_aggregation branch 3 times, most recently from e516c0b to bc69f3e Compare February 29, 2024 11:22
@Stukalov-A-M Stukalov-A-M added the Refactor Improvement to overall code quality label Feb 29, 2024
@Stukalov-A-M
[refactor] hyperledger-iroha#4229: Removed MST aggregation
aef18d2 
Signed-off-by: Stukalov-A-M <stukalov07@gmail.com>
@Stukalov-A-M Stukalov-A-M merged commit 96bf997 into hyperledger-iroha:iroha2-dev Feb 29, 2024
12 of 13 checks passed
@Stukalov-A-M Stukalov-A-M deleted the removed_mst_aggregation branch February 29, 2024 12:55
DCNick3
DCNick3 reviewed Mar 4, 2024
View reviewed changes
core/src/queue.rs
@@ -646,7 +615,7 @@ mod tests {
query_handle,
));
let queue = Queue::from_config(Config {
transaction_time_to_live: Duration::from_millis(200),
transaction_time_to_live: Duration::from_millis(300),
Copy link
Contributor

@DCNick3 DCNick3 Mar 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks the test for me, and I don't think this change can be right without changing the expected numbers of transactions collected

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, i fixed this test in #4319

@s8sato s8sato mentioned this pull request Apr 4, 2024
Merged
5 tasks
@alexstroke1 alexstroke1 assigned alexstroke1 and unassigned AlexStroke Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DCNick3 DCNick3 DCNick3 left review comments

@VAmuzing VAmuzing VAmuzing approved these changes

@s8sato s8sato Awaiting requested review from s8sato

@mversic mversic Awaiting requested review from mversic

@SamHSmith SamHSmith Awaiting requested review from SamHSmith

@outoftardis outoftardis Awaiting requested review from outoftardis

@0x009922 0x009922 Awaiting requested review from 0x009922

@Asem-Abdelhady Asem-Abdelhady Awaiting requested review from Asem-Abdelhady

@horizenight horizenight Awaiting requested review from horizenight

@Arjentix Arjentix Awaiting requested review from Arjentix

@Erigara Erigara Awaiting requested review from Erigara

Assignees

@VAmuzing VAmuzing

@Arjentix Arjentix

@Erigara Erigara

@Stukalov-A-M Stukalov-A-M

@alexstroke1 alexstroke1

Labels
api-changes Changes in the API for client libraries iroha2-dev The re-implementation of a BFT hyperledger in RUST Refactor Improvement to overall code quality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@Stukalov-A-M @coveralls @Erigara @VAmuzing @s8sato @DCNick3 @Arjentix @AlexStroke @alexstroke1